Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use TS CommonJS modules #51

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Use TS CommonJS modules #51

merged 5 commits into from
Jan 17, 2024

Conversation

nickrttn
Copy link
Contributor

@nickrttn nickrttn commented Jan 16, 2024 •
edited
Loading

After some recent build errors in our CI, I think we should use plain CJS here until we support ESM in the API repo. This will make sure TS doesn't inject any compatibility code and compiles to simple CJS.

Docs to understand why nodenext: https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext

And the weird export = ... and import ... = require(...): https://www.typescriptlang.org/docs/handbook/modules/reference.html#export--and-import--require

Before

CJS with compatibility code:

Screenshot 2024-01-16 at 17 04 13

After

Plain CJS

Screenshot 2024-01-16 at 17 03 56

@nickrttn nickrttn requested review from kvz and aduh95 January 16, 2024 16:03
@aduh95
Copy link
Contributor

aduh95 commented Jan 16, 2024

I know this PR is not the right place to discuss this, but since the code for fileExists was taken as an example: shouldn't we deprecate that fileExists util?

  • It can be achieved with built-in node:fs/promises: fs.promises.access(path).then(() => true, () => false).
  • Checking the existence of a file, especially in an async context, is an anti-pattern (by the time you get a response, it might be long-outdated).
  • It's not actually checking if a file exists, but if the program has read access on it.

@nickrttn
Copy link
Contributor Author

Makes sense to me @aduh95. Any feedback on the PR itself?

aduh95
aduh95 approved these changes Jan 17, 2024
View reviewed changes
packages/abbr/src/abbr.test.ts Outdated Show resolved Hide resolved
packages/analyze-step/src/analyzeStep.ts Outdated Show resolved Hide resolved
packages/analyze-step/src/analyzeStep.ts Outdated Show resolved Hide resolved
@nickrttn nickrttn enabled auto-merge January 17, 2024 12:53
@nickrttn nickrttn merged commit a79f33b into main Jan 17, 2024
1 check passed
@nickrttn nickrttn deleted the use-ts-cjs-modules branch January 17, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@kvz kvz Awaiting requested review from kvz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nickrttn @aduh95